Support for JBIG2 (port of https://github.com/apache/pdfbox-jbig2)#338
Support for JBIG2 (port of https://github.com/apache/pdfbox-jbig2)#338kasperdaff wants to merge 1 commit intoUglyToad:masterfrom
Conversation
|
Thanks for this contribution. I'm probably not going to get time to do a review until next weekend, sorry for the delay. |
EliotJones
left a comment
EliotJones
left a comment
There was a problem hiding this comment.
Thanks for all the hard work here and sorry it has taken so long to review, it's a bit large for one person to feasibly review but other than scanning for obvious security flaws I'm not too worried about any style quibbles since it's all pseudo-Java-ish but isolated in the JBIG folders.
The only comments I'd prefer to address prior to merging are the 2 test files with the scary licensing terms and the changes in XObjectFactory.
| @@ -0,0 +1,21 @@ | |||
| ============================================================================ | |||
| The files sampledata_pageN.jb2 and sampledata.jb are included subject to the | |||
There was a problem hiding this comment.
Can we by any chance remove the test files in question please? I'm never sure with copyright and licensing but the non-commercial clause in this license makes me anxious.
| /// <summary> | ||
| /// Interface for all JBIG2 dictionaries segments. | ||
| /// </summary> | ||
| internal interface IDictionary : ISegmentData |
There was a problem hiding this comment.
I would maybe make this a more JBIG specific name, eg. IJbigDictionary since it could easily be confused with relating to the DictionaryToken type.
| /// <returns>A list of <see cref="Bitmap"/>s as a result of the decoding process of dictionary segments.</returns> | ||
| /// <exception cref="InvalidHeaderValueException">if the segment header value is invalid.</exception> | ||
| /// <exception cref="IntegerMaxValueException">if the maximum value limit of an integer is exceeded.</exception> | ||
| /// <exception cref="System.IO.IOException">if an underlying IO operation fails.</exception> |
There was a problem hiding this comment.
I think this is specific to Java code and we wouldn't expect an IOException to be produced unless this is doing some file system access?
| } | ||
| } | ||
|
|
||
| internal static string bitPattern(int v, int len) |
|
|
||
| var width = dictionary.Get<NumericToken>(NameToken.Width, pdfScanner).Int; | ||
| var height = dictionary.Get<NumericToken>(NameToken.Height, pdfScanner).Int; | ||
| var width = dictionary.Get<NumericToken>(NameToken.Width).Int; |
There was a problem hiding this comment.
What's the reason for removing the overload using pdfScanner in this file? In general overloads of Get and TryGet parsing pdfScanner or DirectObjectFinder.Get are required because most tokens in PDF documents can be indirect references to other tokens, in this case an Indirect reference to a number token in its own object.
Usually where they are used it's because there was already a bug in the code which meant they needed to be added, I just want to check we're not causing a regression in this file 🙏
|
I can implement the necessary changes if need be, also saw that it was using |
|
@EliotJones @kasperdaff as it would be sad to lose this work, I will create a new PR by cherry picking the work and adding the changes |
|
Closing PR as it was made available through https://github.com/BobLd/UglyToad.PdfPig.Filters.Jbig2.PdfboxJbig2 |
3 participants
No description provided.